Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: introduce new administrative capabilities #832

Merged
merged 36 commits into from
Aug 9, 2023
Merged

Conversation

travis
Copy link
Member

@travis travis commented Jul 18, 2023

As described in https://hackmd.io/@tvpl/H1whErCI3, introduce a set of capabilities that will:

  1. allow providers to rate limit (especially fully limit, ie block) users
  2. allow providers to get information about subscriptions and consumers

I've taken this opportunity to remove the AccessVerifier from the service and now simply use the real access service implementation for space provisioning.

I'm not entirely happy with the verbosity of the tests for either capabilities or invocation providers, but I'd like to address that in a separate line of work - I'm taking notes!

TODO

As described in https://hackmd.io/@tvpl/H1whErCI3, introduce a set of capabilities that will:
1) allow providers to rate limit (especially fully limit, ie block) users
2) allow providers to get information about subscriptions and consumers

TODO
-[] write tests for capabilities (need to pick @Gozala's brain)
-[] implement in-memory RateLimitsStorage
-[] use it to write `upload-api` tests
-[] move capabilities spec from hackmd to https://github.com/web3-storage/w3up/blob/main/spec/capabilities.md
-[] port 0eea3d1 and d230033 to this commit
@travis travis marked this pull request as draft July 18, 2023 00:32
travis added 16 commits July 18, 2023 16:24
originally implemented in #801, this allows us to block space allocation for a particular space or account creation for a particular email or domain
mostly just cargo-culting the existing type setups, but basing these types on the designs proposed in https://hackmd.io/@tvpl/H1whErCI3
also update existing Consumer.has types to match our normal patterns
it really wasn't happy with getCustomer potentially returning `null` and I think that should use a `Ucanto.Failure` anyway so I elected to make a backwards-incompatible change here since I think this work will merit a breaking change anyway
this eliminates the specter of partial failure
Remove it from the interface - I thought that IN queries would be supported by Dynamo but they aren't. I was already on the fence about this interface method and this pushed me into the "not worth it" camp.
I thought we were doing this, but the implementation of `allocateSpace` in the `AccessVerifier` never actually called into `Space.allocate`, it just used `Space.info` to make sure the space existed, which, until now, fair!

I'm not sure this is the right way to call the `Space.allocate` provider - maybe it would be better to invoke and execute the actual capability rather than just calling the provider function?
travis added a commit to storacha/w3infra that referenced this pull request Jul 28, 2023
Implement the new RateLimitStorage interface from storacha/w3up#832 and integrate it into the upload-api.

This gives us the ability to block uploads to a space and to block an email or domain name from authorizing with our service.
this gets basic testing in place for the new rate limit capability handlers

the test implementations are a bit wordier and boilerplate-inclined than I'm strictly happy with, but I don't have any easy plans to clean them up - need to think and prototype a little
The AccessVerifier was an artifact of the dual-service architecture we had previously, and its implementation in tests
both hid bugs and made it more difficult to test the whole system. I've removed it and gotten the tests running!
@travis travis marked this pull request as ready for review August 2, 2023 01:09
travis and others added 3 commits August 3, 2023 15:47
Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have provided some feedback and suggestions, nothing critical but I think there was one small bug, the rest is kind of take it or leave it.

Either way I'm approving this as I don't think we need another review round to get this out.

packages/capabilities/test/capabilities/rate-limit.test.js Outdated Show resolved Hide resolved
packages/upload-api/src/access/authorize.js Outdated Show resolved Hide resolved
packages/upload-api/src/customer/get.js Show resolved Hide resolved
packages/upload-api/src/provider-add.js Outdated Show resolved Hide resolved

if (allocated.error) {
return allocated
const hasProviderResult = await hasProvider(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not want to check the block list here ?

I also would recommend command style API over defensive style API that is instead of having hasProvider you could have useCapability so that you don't have to have one branch for propagating an error and other branch for creating an error if answer to question was false.

That is also why we had allocateSpace as opposed to `hasSpace.

P.S. It is very same idea as with Parse, don’t validate (a good read if you have not come across it before).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oo I like this, and I also like being able to re-use allocate the way we were - I've pushed a new commit that makes this happen, and drops the size input on space/allocate - we weren't using it for anything and the fact that we don't have a size in the upload/add provider was the main reason I moved away from it.

I suspect we may want to re-introduce it later, either as an optional parameter that triggers a check against a space's allocation or as a separate but similar capability, but this seems ok to me for now - let me know if you think it might be a problem!

22b7a69

packages/upload-api/src/utils/did-mailto.js Outdated Show resolved Hide resolved
packages/upload-api/src/utils/rate-limits.js Outdated Show resolved Hide resolved
packages/upload-api/src/utils/rate-limits.js Outdated Show resolved Hide resolved
packages/upload-api/test/provisions-storage.js Outdated Show resolved Hide resolved
@travis travis changed the title feat: introduce new administrative capabilities feat!: introduce new administrative capabilities Aug 9, 2023
we err on the side of denying a user the ability to authorize or provision a space - they can retry and we really don't want to let people do things unless we're sure they're allowed to
per feedback from @Gozala, move to a style where we always return an `{ok: {}}` result if there is no rate limit above a given threshold.

use this instead of an explicit "block" checker
per feedback from @Gozala, explain the reasoning behind having a separate, opaque identifier for rate limits.
this ensures people can't upload to a blocked space, and makes it more consistent.

this also gets rid of the `size` parameter in `space/allocate` because:

a) we weren't using it for anything
b) we don't have size information in `upload/add`

I think we'll want to add this back in later, possibly as an optional parameter that triggers a check against provisioned allocation, or we'll want to split space/allocate into two capabilities.
@travis travis merged commit 7b8037a into main Aug 9, 2023
4 checks passed
@travis travis deleted the feat/rate-limits branch August 9, 2023 22:45
travis pushed a commit that referenced this pull request Aug 9, 2023
🤖 I have created a release *beep* *boop*
---


##
[9.0.0](capabilities-v8.0.0...capabilities-v9.0.0)
(2023-08-09)


### ⚠ BREAKING CHANGES

* introduce new administrative capabilities
([#832](#832))

### Features

* introduce new administrative capabilities
([#832](#832))
([7b8037a](7b8037a))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
travis added a commit that referenced this pull request Aug 9, 2023
🤖 I have created a release *beep* *boop*
---


##
[5.0.0](upload-api-v4.1.0...upload-api-v5.0.0)
(2023-08-09)


### ⚠ BREAKING CHANGES

* introduce new administrative capabilities
([#832](#832))

### Features

* introduce new administrative capabilities
([#832](#832))
([7b8037a](7b8037a))


### Bug Fixes

* run format for upload-api
([#825](#825))
([59dc765](59dc765))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Travis Vachon <[email protected]>
})

/**
* Capability can be invoked by the provider are an authorized delegate to remove rate limits from a subject.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capability can be invoked by the provider **or** an authorized delegate

>
export type RateLimitRemoveSuccess = Unit

export interface RateLimitsNotFound extends Ucanto.Failure {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: RateLimits is plural here but singular in all other types

if (rateLimitResult.error) {
return {
error: {
name: 'InsufficientStorage',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InsufficientStorage doens't feel like the right name here. It's more of an AccountBlocked kind of error.

travis added a commit to storacha/w3infra that referenced this pull request Aug 11, 2023
Implement the new RateLimitStorage interface from
storacha/w3up#832 and integrate it into the
upload-api.

This gives us the ability to block uploads to a space and to block an
email or domain name from authorizing with our service.

Note that we leave a couple of the new capabilities unimplemented for
now since we don't need them urgently. We are actively hoping to block
some abusive users ASAP, so I'm getting the blocking part of this work
in now and will then turn my attention to the remaining capabilities.

TODO
- [x] remove file dependencies from package.json and point at latest
versions of deps once storacha/w3up#832 is
merged and released
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants